Klib support #183

fzhinkin · 2024-01-31T11:48:21Z

This PR introduces KLib ABI validation support in the BCV plugin.

The brief overview of the design and decisions made is in the corresponding design doc: https://github.com/Kotlin/binary-compatibility-validator/blob/50ebf8b00492d754732e47453b4d8d0351f33baa/docs/design/KLibSupport.md

Providing a programmatic API similar to the kotlinx.validation.api API for working with JVM ABI is planned, but this PR does not include it.

Besides the design doc mentioned above, the main entry points for reviewing are

binary-compatibility-validator/src/main/kotlin/BinaryCompatibilityValidatorPlugin.kt

Line 370 in cac5669

private class KlibValidationPipelineBuilder(

where all the Gradle tasks are configured and

binary-compatibility-validator/src/main/kotlin/klib/KlibAbiDumpFileMerger.kt

Line 77 in cac5669

internal class KlibAbiDumpMerger {

where the current dump merging logic reside.

…r messages

Input klib dump files were not tracked as merger dependencies, so the merged dump was not updated when apiDump was re-executed after sources update

…test

docs/design/KLibSupport.md

src/main/kotlin/klib/KlibAbiDumpFileMerger.kt

sandwwraith

I didn't look at the implementation itself, but I decided to post comments now, because they were pending for too long

src/functionalTest/kotlin/kotlinx/validation/api/TestDsl.kt

sandwwraith · 2024-02-14T17:50:59Z

src/main/kotlin/ApiValidationExtension.kt

+    /**
+     * Specify which version of signature KLib ABI dump should contain.
+     */
+    public var signatureVersion: Int = 2


Why this setting need to be exposed to end users? Can't we infer it from klib file itself?

We can, that's for sure. But when the default/latest version will change, old dumps will be invalid. So there should be a way to specify a particular version.
But here we should probably use some latest-version-available alias instead of the concrete version 2.

I don't get it. If the klib version changes, it means .klib.api file format (not actual signatures) would also change? That looks undesirable. Probably we should have some intermediate parser layer that would provide stable output.

Also, if klib version changes to 3, but user has signatureVersion = 2 it means that we parse klib v3 but output should look like 2. Which brings us again to some middle-layer transformation.

A klib may have multiple signature versions. For example, each declaration may have both v1 and v2 signatures.
The signature is a part of a dump we're producing, so when it changes, the dump also change.

Probably we should have some intermediate parser layer that would provide stable output.

Not sure how we can do that. Signatures are part of a declaration as a linker looks up for symbols by their signatures. And there may be no direct mapping between different signature versions and a declaration they belong to.

Also, if klib version changes to 3, but user has signatureVersion = 2 it means that we parse klib v3 but output should look like 2. Which brings us again to some middle-layer transformation.

We parse the same klib, but instead of dumping v2 signature, will dump v3 signature.

A klib's dump with v1 signature:
https://github.com/Kotlin/binary-compatibility-validator/blob/f5fdacf4d2e9548144f522aa2924742ac4e473f5/src/functionalTest/resources/examples/classes/TopLevelDeclarations.klib.v1.dump
The dump of the same klib using v2 signature:
https://github.com/Kotlin/binary-compatibility-validator/blob/f5fdacf4d2e9548144f522aa2924742ac4e473f5/src/functionalTest/resources/examples/classes/TopLevelDeclarations.klib.dump

So, v3 actually can't link with v2 because they're incompatible? But newer klibs will have both 3 and 2 sets for backwards compatibility?

So, v3 actually can't link with v2 because they're incompatible?

Yes, the linker won't be able to lookup symbols.

But newer klibs will have both 3 and 2 sets for backwards compatibility?

There's no v3 on the horizon right now, so it's hard to tell.

The docs here should guide user when to rely on the default value and when to set it to a fixed value.

Set the null as a default, mentioned that a user should not care about the option by default.

@ilya-g @sandwwraith are you ok with how the version is currently declared and documented?

src/main/kotlin/BinaryCompatibilityValidatorPlugin.kt

sandwwraith · 2024-02-14T18:02:18Z

src/main/kotlin/ApiValidationExtension.kt

+     * By default, ABI dumped only for supported files will be validated. This option makes validation behavior
+     * stricter and treats having unsupported targets as an error.
+     */
+    public var strictValidation: Boolean = false


Shouldn't these values be of type Property<Boolean> to support some Gradle configuration feature? Was it configuration cache or something else, @shanshin ?

Configuration cache seems to work fine with primitive values. But if there's a case where Properties are better, I can change it (note that a parent class, ApiValidationExtension don't use Property too).

Co-authored-by: Leonid Startsev <[email protected]>

… parser

fzhinkin · 2024-03-07T16:54:00Z

What's new:

added public API for managing merged dumps
Gradle tasks were rewritten to use that API
supported dumps generated for klibs with multiple native targets inside (hello, stdlib)
changes KlibTarget serialized form
get rid of some options affecting plugin and the API
fixed dump filter to use java class name format
added tests for the API
updated the design doc

src/main/kotlin/klib/TargetHierarchy.kt

src/main/kotlin/api/klib/KlibTarget.kt

src/main/kotlin/klib/KlibAbiDumpFileMerger.kt

src/main/kotlin/klib/TargetHierarchy.kt

- fixed naming - simplified some code constructions - swapped the order of KlibTarget fields - updated merged dump's header - removed unsupported targets from target hierarchy

…ure of a node being more or less common

src/main/kotlin/api/klib/KlibDumpFilters.kt

src/main/kotlin/api/klib/KlibDump.kt

src/main/kotlin/api/klib/KlibAbiDumpFileMerger.kt

Co-authored-by: ilya-g <[email protected]>

ilya-g

Forgot to submit the last portion of comments

src/main/kotlin/api/klib/KlibDump.kt

src/main/kotlin/api/klib/KlibAbiDumpFileMerger.kt

Added experimental Klib ABI dump validation support. Validation could be performed using the Gradle plugin which provides almost the same validation and dump workflow as for JVM ABI validation. Also, a public API was added to allow users who don't want or can't use the plugin to implement their own workflows. --------- Co-authored-by: Leonid Startsev <[email protected]> Co-authored-by: ilya-g <[email protected]> Pull request Kotlin/binary-compatibility-validator#183

Added experimental Klib ABI dump validation support. Validation could be performed using the Gradle plugin which provides almost the same validation and dump workflow as for JVM ABI validation. Also, a public API was added to allow users who don't want or can't use the plugin to implement their own workflows. --------- Co-authored-by: Leonid Startsev <[email protected]> Co-authored-by: ilya-g <[email protected]> Pull request Kotlin/binary-compatibility-validator#183 Moved from Kotlin/binary-compatibility-validator@ad1bea6

fzhinkin linked an issue Jan 31, 2024 that may be closed by this pull request

Validate KLIB ABI compatibility on Multiplatform #149

Closed

fzhinkin force-pushed the klib-support branch from f80179f to 0a2d44f Compare February 5, 2024 09:50

fzhinkin marked this pull request as ready for review February 5, 2024 11:10

fzhinkin requested review from ilya-g and sandwwraith February 5, 2024 11:10

fzhinkin added the enhancement New feature or request label Feb 5, 2024

fzhinkin requested a review from shanshin February 5, 2024 11:17

fzhinkin added 13 commits February 6, 2024 16:14

Introduce target hierarchy mirroring default template from KMP

3c57419

Implement KLib dump merger

4e50f6d

Added Klib-related settings

ba998f0

Added a more precise Copy task

51f7756

Implemented Klib validation

c824926

Rebased to current dev

cb87220

Added a design doc

91903af

Cleanup

5a6cf13

Mark an experimental API with a dedicated annotation

d58121c

Cleanup

34313b1

Add some kdoc

4bc9d2a

Improved test coverage

a987909

Described a klib validation pipeline, fixed task names, improved erro…

5ba0dbe

…r messages

fzhinkin force-pushed the klib-support branch from cac5669 to 5ba0dbe Compare February 6, 2024 15:14

fzhinkin added 2 commits February 9, 2024 11:54

Fixed klib merge task's dependencies

9d9f1e7

Input klib dump files were not tracked as merger dependencies, so the merged dump was not updated when apiDump was re-executed after sources update

Added shortcuts to reduce the amount of boilerplate code in the klib …

cf65ddf

…test

shanshin approved these changes Feb 16, 2024

View reviewed changes

Fixed a test

3f93275

fzhinkin commented Feb 21, 2024

View reviewed changes

docs/design/KLibSupport.md Outdated Show resolved Hide resolved

fzhinkin commented Feb 21, 2024

View reviewed changes

src/main/kotlin/klib/KlibAbiDumpFileMerger.kt Outdated Show resolved Hide resolved

Store children decls as a map, get rid of a separate cache

6b17087

sandwwraith reviewed Feb 22, 2024

View reviewed changes

Fixed typos

f5fdacf

Co-authored-by: Leonid Startsev <[email protected]>

fzhinkin added 3 commits March 7, 2024 12:11

Renaming and doc update

f51611d

Added public API prototype and switched Gradle tasks to it, fixed the…

9aa5be8

… parser

Improved kdoc and added samples

28cc85a

fzhinkin added 6 commits March 7, 2024 18:35

Update README

68ce7af

Update docs

4f4512e

Update API dump and target aliasing rule

ba9fcf1

Improved test coverage, clarified some corner cases

5e01cf4

Fixed typo in a filename

ba29177

Update the design doc

dd776f2

ilya-g reviewed Mar 11, 2024

View reviewed changes

fzhinkin added 3 commits March 12, 2024 11:28

Addressed review comments and suggestions

489eefa

- fixed naming - simplified some code constructions - swapped the order of KlibTarget fields - updated merged dump's header - removed unsupported targets from target hierarchy

Use a length of path from a target hierarchy root to a node as a meas…

a68f661

…ure of a node being more or less common

Consolidated all klib dump related classes in a single package

20b7fd9

fzhinkin requested a review from ilya-g March 12, 2024 10:58

Use root locale instead of a default one

4b19820

ilya-g reviewed Mar 12, 2024

View reviewed changes

src/main/kotlin/api/klib/KlibDumpFilters.kt Outdated Show resolved Hide resolved

ilya-g reviewed Mar 12, 2024

View reviewed changes

fzhinkin and others added 4 commits March 12, 2024 17:50

Rephrased kdoc

077c7ec

Co-authored-by: ilya-g <[email protected]>

Reworked target group aliasing

c1c9921

Decapitalized L in KLib

081fdb0

Update api dump

4f63bae

ilya-g approved these changes Mar 15, 2024

View reviewed changes

src/main/kotlin/api/klib/KlibDump.kt Outdated Show resolved Hide resolved

src/main/kotlin/api/klib/KlibDump.kt Outdated Show resolved Hide resolved

src/main/kotlin/api/klib/KlibAbiDumpFileMerger.kt Outdated Show resolved Hide resolved

Improved kdoc, cleaned up the code

259ad7c

fzhinkin merged commit ad1bea6 into develop Mar 18, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Klib support #183

Klib support #183

fzhinkin commented Jan 31, 2024 •

edited

Loading

sandwwraith left a comment

sandwwraith Feb 14, 2024

fzhinkin Feb 22, 2024

sandwwraith Feb 22, 2024

fzhinkin Feb 22, 2024 •

edited

Loading

sandwwraith Feb 23, 2024

fzhinkin Feb 26, 2024

ilya-g Feb 27, 2024

fzhinkin Feb 28, 2024

fzhinkin Feb 28, 2024

sandwwraith Feb 14, 2024

fzhinkin Feb 28, 2024

fzhinkin commented Mar 7, 2024 •

edited

Loading

ilya-g left a comment

Klib support #183

Klib support #183

Conversation

fzhinkin commented Jan 31, 2024 • edited Loading

sandwwraith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fzhinkin Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fzhinkin commented Mar 7, 2024 • edited Loading

ilya-g left a comment

Choose a reason for hiding this comment

fzhinkin commented Jan 31, 2024 •

edited

Loading

fzhinkin Feb 22, 2024 •

edited

Loading

fzhinkin commented Mar 7, 2024 •

edited

Loading